Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add API to hook into Windows IOCP loop #1345

Closed
wants to merge 2 commits into from

Conversation

darkwisebear
Copy link

@darkwisebear darkwisebear commented Aug 11, 2020

An API similar to the one of mio 0.6 is now available so that custom objects can be added to the IOCP that is used to implement Poll. Relates to #1047.

This change isn't complete yet but in a draft state as I wanted to gather some feedback on the suggested API.

Since sockets are polled by using a single connection to afd.sys, there is no 1:1 relationship between a user-provided token and the completion key of the IOCP. The implementation therefore uses a slab to keep track of the registered handlers and the mapping of the user-provided token to the reported events.

Other notable things about the change:

  • The Overlapped type now uses a boxed trait object instead of a function pointer. While this is resource-heavier, it also enables writing more clean completion handlers as they can now directy carry state instead of having to do some pointer magic or accessing some sort of global state.
  • There is anew type "Readiness" to convert the completion events into polling events. There is also a similar (equally named) type inside the tests and maybe this type is misplaced inside the Windows module.
  • I still need to write more tests and add them to this PR before it can be accepted. Working on it.

An API similar to the one of mio 0.6 is now available so that
custom objects can be added to the IOCP that is used to implement Poll.
This is a first test to play with the Windows IOCP handler API.
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm only looked at the surface level API, not at the implementation and added a few comment.

Overall I'm wondering if its possible to create something more closely resembling the SourceFd API but for RawHandles. So far we exposed very little of the internals of Mio, e.g. no epoll/kqueue specific types but this exposed quite a lot of IOCP I'm not sure about that.

/// * `Binding` - this type is intended to govern binding with mio's `Registry`
/// type. Each I/O object should contain an instance of `Binding` that's
/// interfaced with for the implementation of the `Source` trait. The
/// `register`, `reregister`, and `deregister` methods for the `Source` trait
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that Binding only has register_handle, how would I implement de/re-register?

Or this is that part simply not implemented yet?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not implemented yet because I'm unsure how that should/could be done as in Windows, there is no way of removing a handle from an IOCP except by closing that handle. So the only thing we should do here is to remove the handler from the list of IO handlers, which should be done by a Drop implementation of that type. reregistration would only support changing the token; I will add both functionalities.

/// I/O operations that are registered with mio's event loop. When the I/O
/// operation associated with an `OVERLAPPED` pointer completes the event
/// loop will invoke the object provided as `callback`.
pub fn new<C: CompletionCallback+'static>(callback: C) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a trait with a single method? Can't we useC: FnMut(&OVERLAPPED_ENTRY) + static?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can't Mio provide that implementation? Or is there special handling per type of handle?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was torn on this too, but I've chosen to introduce a trait because it can be extended in a source-compatible way by adding more trait methods with default impls if we find more cases of hooks that could be costomized by mio drivers (e.g. a "pre-poll hook").

There is a blanket impl for FnMuts as well, so they can be used, see the provided sample test case, which is doing exactly that.

/// The callback will be called once the operation that it is bound to via a call to
/// `Overlapped::new` has been completed and the IOCP is signalled. By returning a `Readiness`
/// value, the callback may deliver a `mio::Event` to the user.
fn complete_operation(&mut self, entry: &OVERLAPPED_ENTRY) -> Option<Readiness>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally not the biggest fan out exposing types from different crates, especially when those crates don't have a stable API.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this happens here: This method resembles the old fn(&OVERLAPPED_ENTRY) from mio 0.6, so except from the OVERLAPPED_ENTRY (which is a stable Windows API struct) we do not expose other types. But maybe I misunderstand your comment.

inner: UnsafeCell<miow::Overlapped>,
#[cfg(debug_assertions)]
canary: u32,
callback: Box<dyn CompletionCallback>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe fn(&OVERLAPPED_ENTRY) -> Option<Readiness> would be better, than we don't have to allocate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct. I've chosen to use a boxed trait here because I was looking at mio_named_pipes which does some pointer magic to get its state back inside the callback functions fore read, write and connect. By using dynamic dispatch, we can avoid that and have more safe code, but if resource usage beats that argument (plus the "future hooks" argument from above) I'd be willing to change this to a pure function pointer.

///
/// Won't actually do anything until `register_handle` has been called during a call to
/// `Registry::register`.
pub fn new(registry: &Registry, token: Token) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you don't require an event::Source on creation of Binding?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I just need the handle, not the whole source. I split this into two parts as I wanted to give the user a chance to store the Binding instance before it starts firing events, so the usual sequence in a driver is:

  • Initialize the Option<Binding> member of your custom struct with None
  • Inside Source::register, you then set it to Some(Binding::new(..))
  • then* give it the handle to work with using register_handle

/// Check whether the provided registry is the same as the one used during creation of the
/// binding.
///
/// IOCP driven sources cannot unregister from their completion port before they get closed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No event::Source in Mio can be registered with another Poll, so this is true for all platforms. See IoSource's selector_id field.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, so this is redundand and can be removed. I'll do that.

/// completed IO operation, this method can be used to mark the associated token as ready, e.g.
/// in case it is necessary to signal an initial readiness before any IO operation has been
/// scheduled.
pub fn inject_event(&self, readiness: Readiness) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about exposing such a function. Is the case you describe in the docs real? I personally never needed that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While working on that PR, I used mio_named_pipes as a sample driver and forward ported it. There were cases (example: Establishing the first read after a successful connection) where in case of an error an immediate readiness flag was raised to be able to deliver the error. I also would love to get rid of that call as it requires a "side channel" for events but I couldn't think of a better solution yet.

///
/// This can be useful when only a shared borrow is held and the overlapped
/// pointer needs to be passed down to winapi.
pub fn as_mut_ptr(&self) -> *mut OVERLAPPED {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't really expose non-Mio types.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, will change that one.

@Thomasdezeeuw
Copy link
Collaborator

@darkwisebear having though about this a bit more I think trying to SourceFd like API first would be the best way forward to make externally created event::Sources (e.g. unix/anonymous pipes). I don't know if that is possible however. If not lets try to minimise the API to begin with, e.g. only expose functions like inject_event once requested.

@darkwisebear
Copy link
Author

@darkwisebear having though about this a bit more I think trying to SourceFd like API first would be the best way forward to make externally created event::Sources (e.g. unix/anonymous pipes). I don't know if that is possible however.

The major problem I see when trying to mimic SourceFd is that IOCP doesn't tell you what operation it was that finished when IOCPs get_many returns an event for a certain token (remember: IOCP is completion-based as opposed to readiness-based). Therefore we cannot (in the general case) deduce from a returned IOCP event what operation might now be available on a certain handle/source. I don't know what happens is if you simply signal read AND write readiness on event arrival, but that just doesn't sound right tbh :)

The only idea I have here is to provide premade OVERLAPPED factories that the user needs to use for the operations that require OVERLAPPEDs so that mio knows what kind of Event has to be generated. However, OVERLAPPEDs will also transport information like bytes transferred and the file offset back to the caller, information that is not available through the current mio event API - which I think is deliberate as other systems like async IO on Unix will return read/written bytes immediately, so this is a non-issue there. However, on Windows you usually at least need to know the number of bytes read/written to update internal buffers, maybe also file positions, depending on the driver.

All in all I'd say I agree with starting with a leaner API but I'm convinced that IOCP and SourceFd won't be a good match and we need a broader, more IOCP specific API like the one from mio 0.6.x that this PR is inspired by. In addition I'm not sure whether mio-named-pipes can be refactored such that this API will be sufficient. But if it turns out to be infeasible, we can still add the required APIs.

@darkwisebear
Copy link
Author

@carllerche @Thomasdezeeuw Is this approach still relevant / desired in any form?

@Thomasdezeeuw
Copy link
Collaborator

@carllerche can you look at this? Seeing as an API akin to SourceFd doesn't seem possible it would require a bit more Windows knowledge.

@Thomasdezeeuw
Copy link
Collaborator

@darkwisebear do you plan on continuing this work?

@darkwisebear
Copy link
Author

@Thomasdezeeuw Since there was no additional feedback I didn't pursue this PR any further but used an internal patched version of mio for my projects. Now there is tokio-rs/tokio#3760 which provides NamedPipe which was the reason I proposed this change. Are there other use cases for such an internal API and would it suffice/have a chance to be integrated like this?

@Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw Since there was no additional feedback I didn't pursue this PR any further but used an internal patched version of mio for my projects. Now there is tokio-rs/tokio#3760 which provides NamedPipe which was the reason I proposed this change. Are there other use cases for such an internal API and would it suffice/have a chance to be integrated like this?

I'm afraid I'm not familiar enough with Windows to answer that.

Also, sorry about the lack of feedback, I've been a bit busy over the last year or so, but I have a bit more time for Mio now.

@darkwisebear
Copy link
Author

I'm afraid I'm not familiar enough with Windows to answer that.

I need to know two things:

  1. Are there more use cases for the proposed API?
  2. Is the API as well as the implementation (at least the aproach, the code itself code needs some love anyway since a lot of code has changed since then) close to a state that would lead to a final merge to master?

The core question now is: How/who can we answer those two questions? I would feel a lot more comfortable continuing to work if those were answered.

Also, sorry about the lack of feedback, I've been a bit busy over the last year or so, but I have a bit more time for Mio now.

No worries, it was meant as an offer to the community in case this was needed, didn't have strong feelings about merging that. If there is still need, we can continue working on it.

@roblabla
Copy link

Are there more use cases for the proposed API?

I can answer this one at least. I subscribed to this issue ages ago, because I was looking to integrate windows Filter Ports (FltGetMessage & co, used to communicate with drivers) into tokio. There are a few more APIs like this (most notably, ones based on DeviceIoControl) that currently cannot be integrated into mio cleanly.

@Thomasdezeeuw
Copy link
Collaborator

Are there more use cases for the proposed API?

I can answer this one at least. I subscribed to this issue ages ago, because I was looking to integrate windows Filter Ports (FltGetMessage & co, used to communicate with drivers) into tokio. There are a few more APIs like this (most notably, ones based on DeviceIoControl) that currently cannot be integrated into mio cleanly.

@roblabla could you maybe describe some (rough) requirement that you would need for Mio to work cleanly, and if this pr would (partially) implement those requirements.

@Noah-Kennedy
Copy link

I'm in a similar situation to @roblabla here.

@Thomasdezeeuw
Copy link
Collaborator

I'm closing this due to inactivity. @darkwisebear if you, or anyone else, want to continue this please don't feel discouraged to do so.

Thank you @darkwisebear for your efforts here.

@Noah-Kennedy
Copy link

Noah-Kennedy commented Nov 11, 2021

@Thomasdezeeuw I have a use case where I need to support some custom sockets on windows. I would be willing to contribute to getting an API like this in place for mio.

@Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw I have a use case where I need to support some custom sockets on windows. I would be willing to contribute to getting an API like this in place for mio.

Sounds good. I can't offer much help with the implementation as I'm no Windows expert, but some do sometimes visit our Mio discord channel so you could ask for help there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants